Repository navigation
Conversation
…losed close() and destroy() close the stream at once, but the native frame parser learns of it on a later turn at the earliest, so it still dispatches the rest of the socket read for that stream. The client streamHeaders handler only checked rstCode, which is 0 after a plain close() or destroy(), and emitted 'response', 'push', 'trailers', 'headers' and 'continue' on the closed stream. Return early when the stream is closed, like the server handler and the DATA path already do.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for one more day.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 1 minute for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 1:50 PM PT - Sep 19th, 2026
✅ @robobun, your commit 330f7b4ecfa3ea5c9c70a20c14391e2d6b69957e passed in 🧪 To try this PR locally: bunx bun-pr 43525That installs a local version of the PR into your bun-43525 --bun |
|
Status: the fix is pushed in this PR (#43525). How I reproduced it: a raw TCP HTTP/2 server answers the request on stream 1 with several frames in one
The same cases are in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/node/http2.ts— A client that destroys a waitForTrailers request inside 'response' can crash with an uncaught ERR_HTTP2_INVALID_STREAM: a WINDOW_UPDATE in the same read still dispatches wantTrailers at src/js/node/http2.ts:5249-5259 on the destroyed stream, its listener calls sendTrailers(), and sendTrailers throws at http2.ts:2368-2370 because the stream is destroyed. Node's onStreamTrailers returns early when the stream is destroyed or closed. Fix: in the wantTrailers handler, return when stream.destroyed, and for a close()d-but-not-destroyed stream call native noTrailers(stream.id) instead of emitting, so the writable side still finishes without invoking user code.Extended reasoning...
Trigger: client.request(headers, { waitForTrailers: true }), writes a body larger than the peer's window, and destroys the stream in its 'response' handler; the server's HEADERS 200 and a WINDOW_UPDATE share one socket read (the PR's own repro harness produces exactly this shape).
The WINDOW_UPDATE releases the queued END_STREAM and native dispatches onWantTrailers (h2_frame_parser.rs:1649) for the stream.
wantTrailers at 5249-5259 only checks the WantTrailer bit; stream.destroyed and stream.closed are ignored. It emits 'wantTrailers'.
The idiomatic listener calls stream.sendTrailers({...}); sendTrailers at 2368 throws ERR_HTTP2_INVALID_STREAM because this.destroyed is true. The throw escapes the native dispatch as an uncaught exception.
Node (lib/internal/http2/core.js onStreamTrailers) returns before emitting when destroyed or closed, so the same user code is safe there.
The dismissal called this pre-existing and out of scope, but this PR introduces the contract 'a closed stream emits nothing' and tests it for four events while leaving the one that ends in a crash. Population:…Verification: pre-existing — acknowledged in diff: the PR description's Notes ("Same family, not in this PR ... wantTrailers (http2.ts:5246). After destroy() in 'response', a WINDOW_UPDATE in the same read releases the rest of a waitForTrailers body, and Bun emits 'wantTrailers' on the destroyed stream. The usual listener calls sendTrailers(), which throws ERR_HTTP2_INVALID_STREAM (uncaught)") states…
|
On the I reproduced it on 1.4.3-canary.1+367d939d9 and on main. The server advertises a 10 byte stream window, the client POSTs 100 bytes with
The stack shows the emit in the client I left it out of this PR. The fix belongs in both |
This branch has not been deployed
1 participant
Problem
node:http2client stream that user code closed or destroyed still emits'response','push','trailers','headers'and'continue'. This happens when the header block arrives in the same socket read as the frame whose handler calledclose()ordestroy(). node v26.3.0 emits nothing.ClientHttp2Session.#Handlers.streamHeaders(src/js/node/http2.ts:5106). It checks onlystream.rstCode, which is 0 after a plainclose()ordestroy(). Both calls send their RST_STREAM from a latersetImmediate, so the native parser still dispatches the rest of the read for that stream.Fix
stream.closedis true.pushToStreamalready drops DATA for a closed stream. In node, nghttp2 ignores the frames of a stream once its RST_STREAM is submitted.test/js/node/http2/node-http2-client-close.test.ts. The 5 new tests fail without the fix and pass under node too. Alsotest/js/node/http2/, the 261 nodetest-http2-*tests,grpc-js.Background
streamHeadersis the JS callback that the native HTTP/2 frame parser calls for each complete header block. It picks the event:'headers'for a 1xx status,'response','push'on a pushed stream, or'trailers'.stream.closedreads theStreamState.Closedbit.close(),_destroy()and the native "stream is fully closed" callback set it.setImmediatecallback runs inside that loop.Notes
Repro. A raw TCP HTTP/2 server answers the request with several frames in one
socket.write(). The client acts in the first event handler. Events recorded on the stream, node v26.3.0 against bun 1.4.3-canary.1+367d939d9 and main at 26e7a4b:'headers':destroy()headers 103,closecontinue,headers 100,response 200'headers':close()headers 103,closecontinue,headers 100,response 200'response':destroy()response 200,closetrailers'response':close()response 200,closetrailers'stream':pushed.destroy()closepush 200With the fix every row matches node.
destroy(err)andclose(code)with a code other than 0 already matched, becauserstCodeis not 0 there. For a pushed stream destroyed with code 0 the late'push'did not need the same read: Bun sends no RST_STREAM for it, so a pushed response that arrives much later also emitted'push'.Differences from node that stay.
close(NGHTTP2_NO_ERROR)while the request body is still blocked on flow control. node submits the RST_STREAM only on'finish', so a response that arrives before'finish'still emits'response'and'data'on node. Bun ends the readable insideclose()and drops that DATA. Before this change Bun emitted a'response'with no body here. Now it emits nothing.close(NGHTTP2_CANCEL)inside a frame handler. node defers that RST_STREAM until the read is done and still emits'response'and'data'. Bun emits nothing, before and after this change.createConnectionreturns a plain Duplex). node queues each event withprocess.nextTickand parses the whole chunk before user code runs, soclose()in'headers'comes after'response'and'trailers'are already queued, and node emits them. Bun now emits nothing afterclose()on every transport. The parity this PR claims is with node over net and TLS sockets.Same family, not in this PR. These handlers also act on a stream that user code closed in the same read. They predate this change. Their fix is not an early return, so each needs its own change and tests:
wantTrailers(http2.ts:5246). Afterdestroy()in'response', a WINDOW_UPDATE in the same read releases the rest of awaitForTrailersbody, and Bun emits'wantTrailers'on the destroyed stream. The usual listener callssendTrailers(), which throwsERR_HTTP2_INVALID_STREAM(uncaught). node returns early for a closed or destroyed stream. A stream that was onlyclose()d still needsnoTrailers()to finish its writable side, so a plain return can hang it.streamErrorandaborted(http2.ts:5022-5038). A peer RST_STREAM in the same read as the user'sclose()overwritesrstCode(node keeps 0) and, for an error code, emitsERR_HTTP2_STREAM_ERRORon a stream that closed cleanly. These handlers hold the only#connections--for the stream, so they cannot return early.altsvc(http2.ts:5304). The session'altsvc'event still fires for a stream that user code closed or destroyed.Covered elsewhere: #42369 fixes late HEADERS on a stream that the client already reset (they leak
#connections, andsession.close()never completes). #43465 refuses a PUSH_PROMISE on a request that user code closed.pushed.close()on a client emits'error'("Invalid stream id") and never'close', so the new push test coversdestroy()only.Tests.
node-http2-client-close.test.tsholds theclose(code)event contract and cross-checks itself under node. The new tests moved the frame helpers of its raw server to module scope and gave the server an optional scripted reply. The reply ends with a PING. The test waits for the PING ACK, so the client has dispatched every frame of the write before the assertion. A session'error'or early'close'rejects both awaited promises.Server side:
ServerHttp2Session.#Handlers.streamHeadersalready returns for a closed stream, and'trailers'afterdestroy()orclose()in the'stream'handler already match node.Not related to this diff, seen locally on a debug build with and without the fix:
h2-conformance.test.ts, "stream release after a queued END_STREAM" fails in some runs (test: deflake the h2 stream-release cases on debug builds #42357 is open for it).grpc-js/test-client.test.ts(100 ms connect deadline) andgrpc-js/test-outlier-detection.test.ts(5 s timeout) fail. They pass on a release build.grpc-js/test-resolver.test.tsandtest-tonic.test.tsneed the network.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http2/node-http2-client-close.test.ts